More ntp servers #4448

christoph-zededa · 2024-11-26T16:40:40Z

allow setting more than one NTP server
allow setting NTP servers by domain (instead of just ip)
allow ignoring NTP servers from DHCP server

pkg/pillar/cmd/zedrouter/networkinstance.go

milan-zededa · 2024-12-04T10:35:25Z

pkg/pillar/cmd/zedrouter/networkinstance.go

@@ -103,6 +137,7 @@ func (z *zedrouter) getNIPortConfig(
 		if port == nil {
 			continue
 		}
+		basicNTPServerIPs, _ := types.GetNTPServers(*z.deviceNetworkStatus, port.IfName)


I think it would be less confusing if getNIPortConfig didn't set NTPServers at all (not even the already resolved IPs) and would leave this to attachNTPServersToPortConfigs.
Then also please add a comment for getNIPortConfig mentioning that NTP servers are set separately.

OhmSpectator · 2024-12-13T15:17:28Z

Argh, I will try to review it soon...

Makefile

pkg/pillar/cmd/zedagent/parseconfig.go

pkg/pillar/cmd/zedagent/reportinfo.go

eriknordmark · 2024-12-17T12:50:07Z

pkg/pillar/cmd/zedrouter/networkinstance.go

+				ntpServers = append(ntpServers, ip)
+				continue
+			}
+			dnsResponses, err := devicenetwork.ResolveWithPortsLambda(


How long time can this call take? Any risk of a watchdog due to it?

The timeout is specified here: https://github.com/lf-edge/eve/blob/master/pkg/pillar/devicenetwork/dns.go#L35

Last week I tested the code with more than 10 unreachable NTP servers and I did not notice the watchdog to get triggered.

Do we run zedrouter without watchdog? - https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/zedrouter/zedrouter.go#L175 as I don't see a agentbase.WithWatchdog(...).

We kick of the main watchdog for each agent in device-steps.sh
touch "$WATCHDOG_FILE/$AGENT.touch"

Wouldn't the delay be if there are potentially unreachable/slow DNS servers and there are 10 NTP servers configured with a domain name that needs to be resolved?

I think, now I understand (better) how this works.
I added a call to pubSub.StillRunning(...): https://github.com/lf-edge/eve/compare/2a16d63887c83fbe4ffe5bec74b3a435b0f7f5f2..b9a29770fc4d1103a007ba1bbe3286172047bf4f

eriknordmark · 2024-12-17T12:57:40Z

pkg/pillar/devicenetwork/dns.go

+func ResolveCacheWrap(resolve func(string, net.IP, net.IP) ([]DNSResponse, error)) func(domain string, dnsServerIP net.IP, srcIP net.IP) ([]DNSResponse, error) {
+	return func(domain string, dnsServerIP net.IP, srcIP net.IP) ([]DNSResponse, error) {
+
+		dnsResponses, found := resolveCache[domain]


Do we resolve these globally?
Suppose wwan0 is a walled garden and it has a DNS server on 10.10.10.10 and eth0 is a different walled garden. Then if the configured ntp server domainname is ntp.corp for both wwan0 and eth0 don't we need to ensure that we do the DNS lookup out the correct network interface?

IP addresses and globally unique and visible domain names don't have that issue, but resolving domain names can't assume that.

In the end chronyd and dnsmasq do not care about the different interfaces, do they?

I made the map key a combination of domain and source ip.

dnsmasq is for the applications doing DNS resolution. We configure chronyd with IP addresses. My concern is about the DNS resolution of the NTP server names. Note that we have been resolving the standard pool.ntp.org without worrying about walled gardens, but as we allow the users to specify domain names for NTP servers they might be using ones which are inside their walled garden(s). I'll take a look at your map approach.

I also added a test for the map approach (perhaps that's easier to understand): https://github.com/lf-edge/eve/pull/4448/files#diff-6ecf3e43642f91236a8006c29a4a1abfe7d1ef210b6381ff39808acd3c64bd90R280

pkg/pillar/types/dns.go

* allow setting more NTP servers through controller * allow setting NTP server by domain instead of just IP address (problem: we only resolve the domains once at the beginning) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Don't use _test for the package name of tests, because this disallows accessing private methods Signed-off-by: Christoph Ostarek <christoph@zededa.com>

which makes pillar ignore NTP servers provided by the DHCP server on that particular interface Signed-off-by: Christoph Ostarek <christoph@zededa.com>

eriknordmark

LGTM but still need to see if we the DNS resolutions of lots of names can take too long for the watchdog.
Kick off tests.

continuously in the background so that updated IP addresses can be used if the edge application is restarted (from controller) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

This will update the network instance if the network config changes; for example during re-activation of the edge app. It is necessary as we want to resolve the IP addresses of the NTP servers again on re-activation of the edge app. Signed-off-by: Christoph Ostarek <christoph@zededa.com>

e.g. run `sudo make TAP=tap0 run` to not autoconfigure the second interface and just let it run as a tap device. You then have to bring your own dhcp server and setup everything manually. Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Need to bump eve-api after following PR lf-edge/eve-api#77 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

to the controller so that the controller knows that EVE can handle domain names and not just IP addresses Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa · 2024-12-17T15:56:08Z

Kick off tests.

Sorry, I saw that too late and force-pushed ...

eriknordmark

Run tests

github-actions bot requested review from eriknordmark, milan-zededa, OhmSpectator, rouming and uncleDecart November 26, 2024 16:41

christoph-zededa force-pushed the more_ntp_servers branch 4 times, most recently from 2b8af39 to 6d83fea Compare November 27, 2024 16:12

github-actions bot requested a review from naiming-zededa November 27, 2024 16:13

christoph-zededa force-pushed the more_ntp_servers branch 4 times, most recently from 6c070b3 to 94174f9 Compare December 2, 2024 18:16

milan-zededa reviewed Dec 3, 2024

View reviewed changes

pkg/pillar/cmd/zedrouter/networkinstance.go Outdated Show resolved Hide resolved

milan-zededa reviewed Dec 3, 2024

View reviewed changes

pkg/pillar/cmd/zedrouter/networkinstance.go Outdated Show resolved Hide resolved

milan-zededa reviewed Dec 3, 2024

View reviewed changes

pkg/pillar/cmd/zedrouter/networkinstance.go Outdated Show resolved Hide resolved

christoph-zededa force-pushed the more_ntp_servers branch from 94174f9 to f3dafaa Compare December 3, 2024 12:58

github-actions bot requested a review from milan-zededa December 3, 2024 12:59

milan-zededa reviewed Dec 4, 2024

View reviewed changes

christoph-zededa force-pushed the more_ntp_servers branch from 5f34b33 to a8b0329 Compare December 4, 2024 13:48

github-actions bot requested a review from milan-zededa December 4, 2024 13:49

christoph-zededa force-pushed the more_ntp_servers branch 4 times, most recently from 8ff6cd8 to cabff85 Compare December 9, 2024 13:32

christoph-zededa changed the title ~~WIP: More ntp servers~~ More ntp servers Dec 10, 2024

christoph-zededa force-pushed the more_ntp_servers branch 3 times, most recently from 5e5507d to abba1ec Compare December 10, 2024 12:31

christoph-zededa force-pushed the more_ntp_servers branch from abba1ec to d401cde Compare December 11, 2024 13:00

christoph-zededa marked this pull request as ready for review December 11, 2024 13:14

rene reviewed Dec 16, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

pkg/pillar/cmd/zedagent/parseconfig.go Outdated Show resolved Hide resolved

christoph-zededa force-pushed the more_ntp_servers branch from e9050d6 to 148acfe Compare December 16, 2024 10:37

eriknordmark reviewed Dec 17, 2024

View reviewed changes

pkg/pillar/cmd/zedagent/reportinfo.go Outdated Show resolved Hide resolved

eriknordmark reviewed Dec 17, 2024

View reviewed changes

pkg/pillar/types/dns.go Show resolved Hide resolved

christoph-zededa added 3 commits December 17, 2024 14:11

pillar: enhance NTP server handling

d98e964

* allow setting more NTP servers through controller * allow setting NTP server by domain instead of just IP address (problem: we only resolve the domains once at the beginning) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

pillar/nireconciler: rename test package

1475717

Don't use _test for the package name of tests, because this disallows accessing private methods Signed-off-by: Christoph Ostarek <christoph@zededa.com>

pkg/pillar: honor NtpServerExclusively option

fc3a06e

which makes pillar ignore NTP servers provided by the DHCP server on that particular interface Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa force-pushed the more_ntp_servers branch from 148acfe to 1930504 Compare December 17, 2024 13:12

github-actions bot requested a review from eriknordmark December 17, 2024 13:12

christoph-zededa force-pushed the more_ntp_servers branch 3 times, most recently from 57ac8b1 to 2a16d63 Compare December 17, 2024 14:12

eriknordmark approved these changes Dec 17, 2024

View reviewed changes

christoph-zededa force-pushed the more_ntp_servers branch from 2a16d63 to b9a2977 Compare December 17, 2024 15:50

github-actions bot requested a review from eriknordmark December 17, 2024 15:51

christoph-zededa added 5 commits December 17, 2024 16:55

pillar/nireconciler: resolve ntp servers

39baf16

continuously in the background so that updated IP addresses can be used if the edge application is restarted (from controller) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Makefile: enable using eth1 as tap device

d05d1d5

e.g. run `sudo make TAP=tap0 run` to not autoconfigure the second interface and just let it run as a tap device. You then have to bring your own dhcp server and setup everything manually. Signed-off-by: Christoph Ostarek <christoph@zededa.com>

Bump eve-api

3f77347

Need to bump eve-api after following PR lf-edge/eve-api#77 Signed-off-by: Christoph Ostarek <christoph@zededa.com>

zedagent/reportinfo: report NTP domain capability

0f862e1

to the controller so that the controller knows that EVE can handle domain names and not just IP addresses Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa force-pushed the more_ntp_servers branch from b9a2977 to 0f862e1 Compare December 17, 2024 15:55

eriknordmark approved these changes Dec 17, 2024

View reviewed changes

eriknordmark merged commit fa93b4d into lf-edge:master Dec 18, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More ntp servers #4448

More ntp servers #4448

christoph-zededa commented Nov 26, 2024 •

edited

Loading

milan-zededa Dec 4, 2024

christoph-zededa Dec 16, 2024

OhmSpectator commented Dec 13, 2024

eriknordmark Dec 17, 2024

christoph-zededa Dec 17, 2024

eriknordmark Dec 17, 2024

christoph-zededa Dec 17, 2024

eriknordmark Dec 17, 2024

christoph-zededa Dec 17, 2024

christoph-zededa Dec 17, 2024

eriknordmark Dec 17, 2024

christoph-zededa Dec 17, 2024

eriknordmark left a comment

christoph-zededa commented Dec 17, 2024

eriknordmark left a comment

More ntp servers #4448

More ntp servers #4448

Conversation

christoph-zededa commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmSpectator commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

christoph-zededa commented Dec 17, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

christoph-zededa commented Nov 26, 2024 •

edited

Loading